-
Notifications
You must be signed in to change notification settings - Fork 574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wrappers/services: RemainAfterExit=yes for oneshot daemons w/ stop cmds #2920
Conversation
systemd complains if you generate a service file for a oneshot daemon that also includes an ExecStop directive but doesn't specify RemainAfterExit=yes. systemd needs this keep track of the service after the command exits. Also adds a test for this. Fixes: https://bugs.launchpad.net/snappy/+bug/1647169
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Let's get a second review before landing this. I've requested a small change for testing but consider this approved with that change in place.
@@ -170,7 +170,7 @@ func RemoveSnapServices(s *snap.Info, inter interacter) error { | |||
|
|||
func genServiceFile(appInfo *snap.AppInfo) string { | |||
serviceTemplate := `[Unit] | |||
# Auto-generated, DO NO EDIT | |||
# Auto-generated, DO NOT EDIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :-)
@@ -204,6 +205,17 @@ WantedBy={{.ServicesTarget}} | |||
restartCond = systemd.RestartOnFailure.String() | |||
} | |||
|
|||
var remain string | |||
if appInfo.Daemon == "oneshot" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thinking aloud. Is there any other condition where we would like to have remain-after-exit set?
wrappers/services_gen_test.go
Outdated
daemon: oneshot | ||
` | ||
|
||
info, err := snap.InfoFromSnapYaml([]byte(yamlText)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have snaptest.MockYaml
from snap/snaptest
for this purpose. Please use it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zyga Do you mean snaptest.MockSnap
? I don't see a MockYaml
anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant MockInfo :-) They differ subtly but I think all you need is MockInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, please do merge master into this branch, that should fix the test failure.
…neshot-daemons
systemd complains if you generate a service file for a oneshot daemon
that also includes an ExecStop directive but doesn't specify
RemainAfterExit=yes. systemd needs this keep track of the service after
the command exits.
Also adds a test for this and fixes a typo in the opening comment of the generated systemd unit file.
Fixes: https://bugs.launchpad.net/snappy/+bug/1647169